parity: per-service AWS audit of all 163 services - #2452
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 2429 files, which is 2329 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2429)
You can disable this status message by setting the |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The tool already modelled handlers that override a sentinel's mapping, but only when the override helper contains the wire code as a literal in its own body. iot's respondAsConflictCode takes the code as a parameter, so firstCodeLiteral found nothing and the helper was silently dropped from the override set. The tool then fell through to the general ErrAlreadyExists -> ResourceAlreadyExistsException mapping at the backend sentinel and reported six findings for ops that already emit their declared code. The fix reads the code from the argument at each call site rather than from the helper body, which is necessary because the six sites pass different literals -- ConflictException and TaskAlreadyExistsException. maxEmitHop is untouched. Raising it was the obvious move and the wrong one: the same one-hop limit is what produces several other false-positive classes, and widening it risks over-attributing sentinels to ops that cannot reach them. Recognising the shape is narrower and does not trade one noise class for another. The measurement matters more than the fix. A repo-wide scan for this shape found exactly one function: iot's respondAsConflictCode. So this is 60% of one service's findings but roughly 4% of the corpus, not the dominant source of noise it looked like from iot alone. That is worth knowing before deciding how much more of the corpus is worth triaging. iot drops from 10 findings to 4, exactly the six override sites, and no finding appeared anywhere else -- the diff is identical outside the iot section. A codedeploy shift was traced to that package's concurrent edits by controlled A/B revert, not to this change. Three tests guard the result: the override is suppressed, a sibling op without the override is still flagged, and an override whose own code is undeclared is still reported. The last is the one that matters -- an override is not automatically correct. Closes gopherstack-il42 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
All six class A findings are one root cause at three sites: CreatePermission, DeleteCertificateAuthority and ListCertificateAuthorities emit InvalidArgsException, which none of them declares. ListCertificateAuthorities declares exactly one error, InvalidNextTokenException, so there is not even a plausible near-miss. ErrInvalidArgs is correct for other acmpca ops, so this is not a wrong sentinel to swap -- it is the third real-bug shape: a code with no home in these ops' models, and no declared code that fits a validation failure. PARITY.md already recorded this twice, under the 2026-08-31 error-envelope sweep and its post-reachability re-run, both concluding the same thing and leaving landmine comments at all three sites. This pass re-derived it independently from the SDK and agrees. Three independent confirmations is enough. The value here is the confirmation itself, not the absence of a fix: it establishes the earlier conclusion was reasoned rather than an oversight. Nothing shared with services/acm, despite the adjacent name -- no import in either direction, so acm's audit carries across nothing. Closes gopherstack-qrnq Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Four class A findings: three false positives and one real, which is left unfixed because both remedies need evidence this pass did not have. Three are the guard-cannot-fire class. CreateActivity and CreateStateMachine trip does-not-exist guards on ARNs created moments earlier in the same request, under the same lock. StartSyncExecution re-parses a definition that CreateStateMachine and UpdateStateMachine already rejected if unparseable, and every path to a runnable definition traces back to one of those validated writes. All three are already covered by green tests asserting success. The real one is DescribeStateMachineForExecution returning StateMachineDoesNotExist. It declares ExecutionDoesNotExist, InvalidArn and three KMS codes, and none fits the actual condition -- the execution exists and its state machine does not. The branch fires after a restore, because executionDefinitions is deliberately excluded from persistence. Filed as gopherstack-s9zy; the site carries a comment naming both candidate remedies, since persisting the definitions would bump the snapshot version and converting the error to a synthetic 200 needs its own evidence. Two things the tool did not flag are worth recording. DELETING is never observable here -- DeleteStateMachine sets the status and deletes the record in the same locked region -- so StateMachineDeleting can never be emitted and CreateStateMachine's duplicate-name guard tests dead state (gopherstack-kx95). And this service resolves only 37 of 205 ops, so the audit covers 18% of it and cannot be read as a clean bill of health (gopherstack-2kud). Comment and documentation only; no behaviour changed. Closes gopherstack-2hdk Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
gopherstack-8h57 was filed from the 2026-08-31 sweep's deferral note, which recorded that handleAddTags and handleRemoveTags discarded the backend error and always returned 200. That was true when written and is not true now: cff5010 fixed both on 2026-09-04 under gopherstack-to9j, and it is an ancestor of this branch. The filing missed it because the very next PARITY.md section already recorded the fix. Screening a service for a prior sweep is not enough -- the sweep's own deferrals may have been picked up later in the same file. Re-verified rather than taken on trust. AddTags declares BaseException, InternalException, LimitExceededException and ValidationException; RemoveTags the same minus LimitExceededException. Neither declares a not-found code, so ValidationException is the right mapping, and it matches services/opensearch's fix for the same sibling API. Neutering either guard reverts the op to 200 for an unknown ARN and fails its subtest, so the existing coverage has teeth. handleListDomainNames is left alone deliberately. Its skip-on-error only ever runs on names ListDomainNames itself just returned, so a 404 there means a delete raced between the two calls -- a genuine race, not a caller naming something that never existed, and defensible as written. Documentation only; the deferral note now points forward at the fix. Closes gopherstack-8h57 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
…ma screening rule
Four class A findings, no functional change. Three are false positives and one is a real mismatch with no clear remedy. The three -- DescribeACLs, UpdateACL and DescribeUsers -- all reach a shared helper through a discarded-error call of the form allClusters, _ := DescribeClusters(ctx, ""), where the name filter is empty so the not-found guard cannot fire. Existing tests already exercise that exact call shape against a populated store and pass, so the reasoning is not merely theoretical. Not class 8: nothing real is being swallowed, the guard simply has nothing to report. The fourth is CreateCluster emitting SnapshotNotFoundFault, which it does not declare. The model is oddly asymmetric here -- it declares a not-found fault for every other referenced resource, ACL, parameter group, subnet group and multi-region cluster, but none for the snapshot. That asymmetry is why this is filed rather than guessed: InvalidParameterValueException is the plausible answer and the landmine comment names it, but plausible is not the bar. Filed as gopherstack-2i0c. The new test pins the current wrong code on purpose and says so in its own comment, naming the mismatch and the landmine. Five services in this campaign had tests asserting wrong codes with no such note, which is how those defects survived earlier passes; a pinning test that does not admit what it pins becomes the next pass's false evidence. memorydb resolves 45 of 45 ops, so unlike stepfunctions this result covers the whole service. Closes gopherstack-me2v Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Three class A findings across full coverage (29 of 29 ops resolved). One real bug fixed, one false positive, one real mismatch filed. DeleteLoadBalancer returned LoadBalancerNotFound for a missing load balancer. Its deserializer declares no error at all beyond UnknownError, and the doc comment says why: "If the load balancer does not exist or has already been deleted, the call to DeleteLoadBalancer still succeeds." (api_op_DeleteLoadBalancer.go) That is a semantic sentence about not-found behaviour, of the kind that justified the workmail fix -- not the response-shape boilerplate that made codepipeline's three look fixable and were not. It now returns success. Two pre-existing tests asserted the 400 with no note that it was unverified, which is the pattern that let this survive earlier passes. Both now assert success and are renamed to say what they pin. CreateLoadBalancer's finding is a false positive: the hit is AddTags' own not-found guard, reached from the post-create inline-Tags call, on a load balancer created moments earlier in the same request. DeleteLoadBalancerPolicy emits PolicyNotFound and declares only InvalidConfigurationRequest and LoadBalancerNotFound. Copying the fix above would be the obvious move and the wrong one -- that op has no equivalent doc sentence, in either the pinned comment or the live reference. Filed as gopherstack-39ip; its pinning test now carries a disclaiming comment. elbv2 implements these ops independently, so nothing is shared. Closes gopherstack-5gfl Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Four class A findings, all real, and three share a cause worth naming: a
prior error-mapping pass introduced one global sentinel-to-code rule and
applied it uniformly across ops whose declared catalogs differ.
CreateProtectionGroup and TagResource raised ErrSubscriptionRequired,
mapped to InvalidOperationException, which neither
declares. Both declare ResourceNotFoundException,
which also matches DescribeSubscription's real
no-subscription behaviour.
UpdateProtectionGroup raised ErrLimitExceeded for its member cap, mapped
to LimitsExceededException, which it does not
declare -- though CreateProtectionGroup's identical
check does. Now InvalidParameterException.
ListAttacks chained the shared pagination sentinel to
InvalidPaginationTokenException, which it alone
among its pagination siblings does not declare.
The Create/Update contrast is the whole point: the same check on two ops in
one file needs two different codes, which a table keyed by sentinel cannot
express. Filed as gopherstack-hdvu, since most services map sentinels this
way and the fix is per-call-site rather than per-sentinel.
A pre-existing test pinned the wrong code for the member cap with no note
that it was unverified -- the seventh such test this campaign. It now asserts
the right code, that the wrong one is absent, and that a rejected update
leaves the members unmutated.
ListAttacks still shows as a finding after the fix. The call site checks the
shared helper's sentinel and returns its own, which the tool's one-hop trace
cannot see; the new handler-level test proves the emitted code is right.
Coverage is worth recording: shield resolves 13 of 36 ops before this and 11
after, so the audit covers about a third of the service and emission coverage
is not a progress metric.
Closes gopherstack-g2l5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Three class A findings across full coverage (79 of 79 ops resolved). Two are real and share the shape gopherstack-hdvu describes: one sentinel reused across ops whose declared sets disagree. CreateCommit's identical-content check raised ErrSameFileContent, mapped to SameFileContentException. CreateCommit declares NoChangeException and not SameFileContentException; PutFile declares the opposite. The sentinel was correct for PutFile all along, so the fix is a new ErrNoChange used only by CreateCommit rather than a change to the shared mapping. GetCommit returned ErrCommitNotFound, mapped to CommitDoesNotExistException. GetCommit declares CommitIdDoesNotExistException instead -- a genuinely different code, for "the specified commit id does not exist" rather than "no commit specified and the repository has no default branch". ErrCommitNotFound stays correct for CreateBranch and the merge family, so again a new sentinel scoped to the one call site. Both fixes are per call site. Changing either sentinel's table row would have broken the ops it was already right for, which is exactly the failure mode hdvu records. A pre-existing test asserted CreateCommit's wrong code with no note that it was unverified -- the eighth this campaign. It now asserts NoChangeException and is renamed to say what it pins. BatchGetCommits is a class-1 false positive: its failures are per-entry document data in a 200 response, not top-level exceptions. Whether the value it puts there is right is a separate and unevidenced question, filed as gopherstack-pfyr rather than inferred from GetCommit's parallel. Closes gopherstack-8pe4 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
…er declare it Three class A findings across full coverage (8 of 8 ops resolved), all real, all one cause: a required-field check that is correct for the five ops declaring InvalidRequestException was copied onto three that do not. GetResourceRequestStatus declares only RequestTokenNotFoundException, and CancelResourceRequest only that plus ConcurrentModificationException. Neither declares InvalidRequestException, so the empty-RequestToken guard is removed and an empty token now falls through to the same not-found path an unknown token already took. ListResourceRequests is the sharper case: it declares no errors at all beyond UnknownError. An op whose model declares nothing cannot legitimately reject input, so validateFilter is deleted rather than remapped, and an unrecognized filter value returns 200 with zero matches. eventMatchesFilter's existing containment checks already fail such values closed, so nothing is newly accepted -- the response simply stops being an error the model does not have. That change is a 400 becoming a 200, which normally needs its own evidence beyond a declared-set mismatch. Here the mismatch is the evidence: there is no declared code to move to, and zero declared errors is a positive statement about the op rather than an omission. A pre-existing test asserted the 400 for unrecognized enum values -- the ninth such test this campaign -- and now asserts 200 with zero matches, plus a real match for valid values so the filter is still pinned in both directions. None of the three belongs in a ProgressEvent: they are request-shape checks reaching handleError directly, not provisioning outcomes, so this is unrelated to the synchronous-completion divergence recorded for this service. Findings drop from 3 to 0. Closes gopherstack-v5eb Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Two class A findings: one real, one a false positive. ListDeliveryStreams declares no exception at all beyond UnknownError, so it cannot legitimately reject input. It was rejecting an unrecognized DeliveryStreamType filter with InvalidArgumentException. The validator is deleted rather than remapped -- there is no declared code to move to, and zero declared errors is a positive statement about the op. An unrecognized value now matches no stored stream, which is what any other unmatched filter value already did. Same shape as cloudcontrol's ListResourceRequests. Worth noting the validator was also wrong on its own terms: it accepted all four DeliveryStreamType values, so a filter on MSKAsSource or DatabaseAsSource had previously errored rather than returning an empty list. Deleting it fixes both problems at once. CreateDeliveryStream's finding is a class-4 false positive. The sentinel references are TagDeliveryStream's and StartDeliveryStreamEncryption's own not-found guards, and both of those ops do declare ResourceNotFoundException. handleCreateDeliveryStream calls them with the name it has just created, so the guard cannot fire -- which is presumably why CreateDeliveryStream's own declared set has no such code. A pre-existing test asserted the 400 for a bogus filter with no note that it was unverified -- the tenth this campaign. It now asserts 200 and that the bogus filter matches neither existing stream, so the filter stays pinned. Coverage is worth recording: firehose resolves 12 of 124 ops, so this audit covers 10% of the service and the tool flags it UNVERIFIED. Two findings clean is not a clean service. Closes gopherstack-t2wb Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Two class A findings, both real, both a Describe/List op treating a filter as a must-exist key. DescribeRepositoryCreationTemplates declares InvalidParameterException, ServerException and ValidationException. It emitted TemplateNotFoundException for an unmatched Prefixes entry. Its siblings DeleteRepositoryCreationTemplate and UpdateRepositoryCreationTemplate do declare that code, which is the tell: the delete and update paths key on a template, the describe path filters on one. An unmatched prefix is now omitted rather than fatal. ListImageReferrers declares RepositoryNotFoundException but not ImageNotFoundException, and it emitted the latter for an unknown subject digest. The repository guard stays and is now pinned by its own test; only the subject-image guard goes. That also fits this service's already-recorded gap that referrer edges are unmodelled, so real AWS has nothing to validate the subject against here. Same shape as rds DescribeDBClusterEndpoints under gopherstack-33jc, where an optional filter was likewise treated as a key. Worth watching for on any Describe or List op. Two pre-existing tests asserted the wrong behaviour with no note that it was unverified -- the eleventh and twelfth this campaign. Both now assert 200 with an empty list and are renamed to say what they pin. Findings drop from 2 to 0. Closes gopherstack-jqxg Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Two class A findings, both the same shape and both dismissed. BatchGrantPermissions and BatchRevokePermissions build an errCode per entry and return it inside BatchFailureEntry.Error.ErrorCode, in a 200 response body. That is document data, not a top-level exception, so the op's declared set does not constrain it. types.ErrorDetail.ErrorCode is a bare *string with no enum, which confirms AWS models it as free-form. The extraction is worth recording even though it does not decide the case: both ops declare InvalidInputException and OperationTimeoutException and not InternalServiceException, so the flagged code genuinely does not appear in their exception lists -- it simply lives in a different layer. Reading the mismatch as a bug would have been the same error as sqs's ChangeMessageVisibilityBatch under gopherstack-opzq. Neither is the filter-as-key shape that accounted for four of the last six real findings; both are mutate ops. One thing noticed in passing and left alone: the InternalServiceException branch is unreachable today, since every error path in grantPermissionsLocked and revokePermissionsLocked wraps ErrValidation and so takes the InvalidInputException branch. Defensive rather than wrong, and the field is unconstrained either way. lakeformation resolves 61 of 61 ops, so this covers the whole service. No code changed. Closes gopherstack-4lvy Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
…uses Two class A findings, both real. One is fixed and one is filed, and the split is the point. ListGroupingStatuses declares no NotFoundException while its sibling ListGroupResources, keying on the same required Group member, does. It is a List op, so an unknown group now yields an empty result rather than a 404 -- the same forced semantics as rds DescribeDBClusterEndpoints, ecr ListImageReferrers and firehose ListDeliveryStreams. Three of its tests asserted the 404 with no note that it was unverified. CancelTagSyncTask has the identical mismatch and is deliberately left alone. This pass first fixed it the same way, and that was wrong: a List op's remedy is forced because there is nothing to return but an empty list, while a mutate op's is not. Neither candidate survived scrutiny. There is no idempotency language for it in the live reference or in botocore, only the generic empty-body boilerplate -- which appears verbatim on this service's own GetGroup and DeleteGroup, both of which declare NotFoundException and do error. That is the control experiment that disproved the same boilerplate for codepipeline, repeated inside this service. BadRequestException is declared and arguably fits, but nothing establishes unknown-resource to BadRequest as an AWS pattern. So CancelTagSyncTask keeps its current behaviour with a landmine comment naming both candidates, and its tests are restored byte-for-byte, including the region-isolation test that proves isolation via the not-found error rather than the weaker no-op assertion the abandoned fix had required. Filed as gopherstack-t3uf. Findings drop from 2 to 1. Closes gopherstack-m4k0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
This branch's 98bdada ("ses,sts: emit declared error codes, not invented ones") changed STS's missing-EncodedMessage rejection from the fabricated "InvalidParameter" to "MissingParameter". It updated the unit test in services/sts/ but missed this integration copy, which runs in a separate CI job the per-package gates never exercise, so integration-tests (0) and (1) failed: test/integration/sts_test.go:515 "...api error MissingParameter: EncodedMessage is required..." does not contain "InvalidParameter" Re-verified the ground truth independently rather than editing the assertion to match whatever the code emits, since that is the defect class 98bdada set out to remove. From botocore's sts/2011-06-15 model: DecodeAuthorizationMessage.errors = [InvalidAuthorizationMessageException] "The error returned if the message passed to DecodeAuthorizationMessage was invalid. This can happen if the token contains invalid characters, such as line breaks, or if the message has expired." That is malformed content, not a missing parameter, and it remains what authorization_message.go returns for the content path. Neither "MissingParameter" nor a bare "InvalidParameter" appears anywhere in the STS model or in sts@v1.45.4 — "MissingParameter" is a generic Query-protocol frontend code, listed as such in cmd/errtargetaudit/genericcodes.go:28, which is the correct bucket for a missing required member. The handler maps ErrMissingEncodedMessage alongside twelve sibling ErrMissingXxx sentinels. Swept test/integration/ for other assertions the four gopherstack-yatn clusters could have invalidated: bare "InvalidParameter" had exactly this one hit, and the SES half of that commit (FilterDoesNotExist) has none. Gates: golangci-lint ./test/integration/... 0 issues; go vet -tags integration clean; package compiles. Closes gopherstack-fahz Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
unit-tests (0) failed with:
=== FAIL: cmd/errcodeaudit TestScanServiceDir_PersonalizeInternalServerExceptionReported
scan_test.go:326 Error: "0" is not positive
cmd/errcodeaudit and cmd/errtargetaudit read pinned SDK modules straight off
disk as ground truth without importing them. The personalize SDK is imported
only from test files, and the round-robin shard (NR % 4 over `go list ./...`)
put cmd/errcodeaudit in chunk 0 and services/personalize in chunk 2, so
chunk 0 never built anything that pulled the module, never downloaded it,
and the scan came back empty. Merging main added packages, which reshuffled
every assignment and split the two apart; the test and the go.mod pin are
both unchanged.
Chose to make the module present rather than skip the assertion. The
sywi precedent (skipUnlessSDKModuleCached in errtargetaudit) would skip
*probabilistically* here, not permanently: unlike schemas, which no non-test
file imports at all, personalize's module does get downloaded whenever a
package importing it lands in the same chunk. That is a coin flip that
re-rolls on every package-count change, so a skip guard would leave the
gopherstack-oshm regression check running only sometimes.
`go mod download` with no arguments fetches every module go.mod explicitly
requires — 331 of them here, including personalize, personalizeruntime, ecs
and schemas, all direct requires. Verified with `go mod download -json`.
That decouples module presence from chunk membership for the whole class,
and it also makes the existing errtargetaudit schemas skip stop firing, so
that assertion starts running in CI as well.
Reproduced the failure against a mirrored GOMODCACHE missing the module
(exact match to CI), then confirmed `go mod download` repairs it and the
test passes.
Found a second latent instance of the same class while sweeping:
TestScanServiceDir_ECSValidationBar is fragile identically and is masked
today only because cmd/errcodeaudit and services/ecs happen to share chunk
0. It breaks the same way when ecs's module is absent, and this fix covers
it too.
The round-robin sharding still makes any GOMODCACHE-dependent test
order-fragile in principle; this makes that moot for this class rather than
removing it.
Gates: golangci-lint ./cmd/errcodeaudit/... 0 issues; go test -race and
-race -shuffle on -short both ok; ci.yml parses and the step lands between
Set up Go and the shard computation, uniform across all four chunks.
Closes gopherstack-w052
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
integration-tests (2) and (3) failed on three unrelated tests. None is a regression and none involves the IAM/STS enforcement changes — each is an integration test missed when a parity fix landed on Sep 4 and updated the equivalent unit tests but not the integration suite. apigateway (expected 404, got 403): real API Gateway answers any unmatched execute-api resource path with 403 "Missing Authentication Token", not 404, independent of authentication. 6bb4f25 implemented that in proxy.go and corrected four tests that had codified the wrong status; this one was not among them. services/apigateway/proxy_test.go:626 already asserts 403 for the same scenario. servicediscovery (200/400 apparently inverted): not an inversion. botocore servicediscovery/2017-03-14 declares DeleteServiceAttributesRequest required members as [ServiceId, Attributes], and GetServiceAttributes declares only InvalidInput and ServiceNotFound — there is no "no attributes" error. 50bdbac made DeleteServiceAttributes honour the Attributes key list instead of silently wiping everything. The test never passed Attributes, so the delete correctly 400s, the attributes survive, and the follow-up Get correctly returns 200. Adds a delete-all-then-get unit case. timestreamwrite (RejectedRecordsException "version conflict"): the message was a red herring. RejectedRecordsException has three documented causes, and RejectedRecordsError.Error() hardcoded "due to version conflict" for all of them. The real cause was a stale fixture timestamp (1609459200000, Jan 2021) outside the memory-store retention window — 0b15890 added that rejection and moved about thirty tests inside the window, missing this one. Drops the misleading clause from the generic message; the per-record Reason still carries the specific cause, and nothing asserted the old string. Gates: golangci-lint on the three service packages plus ./test/integration/ 0 issues; go test -race on all three ok. Closes gopherstack-oc21 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
CloudTrail, Textract, EFS and ELB integration tests failed with
S3BucketDoesNotExistException, InvalidS3ObjectException, SubnetNotFound and
InvalidInstance. Not a regression: the tests asserted against resources they
never created, using fabricated identifiers.
efs_test.go:123 SubnetId: aws.String("subnet-12345678")
elb_test.go:26 instA = "i-0a0a0a0a0a0a0a0a0"
cloudtrail_test.go:23 s3Bucket := "test-bucket-"+uuid[:8] (never created)
textract_test.go Bucket: aws.String("it-textract-bucket")
The cross-service validations that reject them are branch-new and correct.
wireCloudTrailS3, wireTextractS3 and wireEFSCrossService are absent from
main's cli.go; wireELBCrossService exists there, but the
b.ec2Resolver.InstanceExists loop in RegisterInstancesWithLoadBalancer is
new. That is why these jobs pass on merged PRs — main has no such check, so
fabricated IDs sail through. Four services failing at once is coincidence of
timing, not a shared cause.
Two earlier hypotheses were investigated and refuted. The
siblingServices/SetAppConfig mechanism is not used by any of the four; they
take narrow S3Backend/EC2Resolver interfaces injected directly from cli.go.
IAM/STS enforcement is not involved: these are in-process Go calls that never
reach EnforcementMiddleware, and the observed codes are AWS-declared business
errors, not AccessDenied.
Each test now creates its prerequisite through the real API — CreateBucket,
CreateBucket+PutObject, CreateVpc+CreateSubnet, RunInstances — and uses the
returned identifiers instead of literals, with cleanup registered so it runs
after the dependent resource is torn down. Textract now generates a unique
bucket per test rather than sharing one literal name across two parallel
tests, since s3 CreateBucket errors on a repeat create.
No production code changed and no validation weakened.
Not executable here: these need Docker, so HTTP dispatch, SigV4 and
container startup stay unverified. The API sequences were matched against
each service's own cross-service unit tests (cloudtrail/s3_delivery_test.go,
textract/s3_object_test.go, efs and elb crossservice_test.go) instead.
Gates: golangci-lint ./test/integration/... 0 issues; go vet -tags
integration clean; package compiles.
Closes gopherstack-3vif
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
terraform-tests (3), (5) and (6) failed on this branch and on Sep 8, before the merge; shards 0,1,2,4,7 pass. All three are stale fixtures meeting correct, recently-added validations — no production code is wrong. The "waiting for SQS Queue" line appearing in all three shards is teardown collateral, not a shared cause: none of the three fixtures references SQS, each runTFTest gets its own t.TempDir() and isolated apply/destroy, and each shard also runs a genuine SQS test in parallel writing into the same interleaved -v log. Same for the Beanstalk and ECS lines alongside (6). elbv2: ee559f1 added elbv2.EC2Resolver/validateNetworkRefs, wired live in cli.go, so CreateLoadBalancer now checks SubnetExists. The fixture used fabricated subnet-00000001/2 and vpc-00000000 with no matching resources — real AWS rejects that too. Adds aws_vpc and two aws_subnet, wiring their real ids into aws_lb.subnets and the target group's vpc_id. textract: handleDetectDocumentText -> checkS3Object -> HeadObject validates the referenced object exists. The fixture ran the CLI against a bucket and key it never created, so the provisioner exited nonzero. Adds aws_s3_bucket + aws_s3_object with depends_on; the Go verify step reuses the same bucket/key. apigatewayv2: the emulator is right and the fixture was wrong. botocore's CreateIntegrationRequest.IntegrationType doc says verbatim "MOCK: ... Supported only for WebSocket APIs", which is exactly what validateIntegrationTypeForProtocol enforces. The fixture built an HTTP API with a MOCK integration. Switched to HTTP_PROXY with integration_method and integration_uri; the verify step only checks GetApis lists the API. Not executable here — no Docker. Correctness rests on reading the live-wired validation paths, the botocore model, and matching fixture patterns already proven in this repo. Unverified: end-to-end apply, the AWS provider's client-side required-field combo for HTTP_PROXY, and aws_s3_object content round-tripping through the emulator's S3 path. Gates: golangci-lint ./test/terraform/... 0 issues; go vet clean; package compiles; tofu fmt -check clean. Closes gopherstack-w8ka Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Preventive, plus one test that was already broken. gopherstack-3vif fixed four integration tests that asserted against resources they never created, once branch-new cross-service validation started rejecting fabricated identifiers. The same pattern remained in eks, fsx, mwaa and resiliencehub, passing only because those services do not validate subnets yet. When any gains an EC2Resolver.SubnetExists check — exactly what EFS just got — they would fail identically, and the failure reads as a cross-service wiring regression rather than a stale fixture. That misreading cost real time on 3vif. Each now creates a real VPC and subnet through ec2Client and uses the returned id, following efs_test.go's cleanup ordering so subnet/VPC teardown runs after the dependent resource. CIDRs use 172.20-172.29 because CreateVpc rejects overlapping ranges across concurrently-live VPCs and every 10.x range was already taken by other parallel tests. ALREADY BROKEN, not latent: TestIntegration_MWAA_InvokeRestApi's two non-error subtests and TestIntegration_MWAA_PublishMetrics's one call CreateEnvironment with no NetworkConfiguration at all, then require.NoError. validateNetworkConfigCreate (services/mwaa/validation.go:203) rejects a nil NetworkConfiguration unconditionally, so those subtests fail every run. MWAA's own unit tests were updated to always send one; the integration test was left stale. All three call sites now send 2 real subnets. Deliberately not changed: sg-12345678 in mwaa (only the count is validated, 1-5, and no EC2Resolver exists for security groups) and ami- literals everywhere (services/ec2 has no AMI registry at all — structural, not a per-service latent bug). resiliencehub's "i-doesnotexist" literals are intentional not-found fixtures. Swept the rest of test/integration/ and recorded the findings in gopherstack-1o31: subnet- literals remain in apigatewayv2, dax, kafka, rds, route53resolver, vpclattice and grafana tests, all same-shape risk; elbv2_test.go uses vpc-00000001 while already creating real subnets in the same file; lambda_new_ops_test.go is not at risk since it runs against an isolated in-process backend that cli.go's wiring never reaches. Not executable here — no Docker. Verified by reading each service's validation for format/count/existence checks that could reject the new inputs, and by compile, vet and lint. Gates: golangci-lint ./test/integration/... 0 issues; go vet -tags integration clean; package compiles. Closes gopherstack-1o31 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
unit-tests (1) intermittently failed on "server did not shut down within timeout" (cli_test.go, eight sites). Pre-existing: it failed on Sep 8 before the merge, with two different tests hitting it in one run. Root cause: shutdownTimeout (5s) was re-armed three times in sequence. startServer's server.Shutdown got a fresh 5s; lambdaCloseFn got another; shutdownServices via shutdownBackends got a third. All three run sequentially — stages two and three execute in run()'s deferred shutdownBackends, after startServer has already returned — so worst-case teardown could take ~15s while every test budgeted 5s. Captured evidence: "Shutting down server..." logged at 00:34:34.813, the first "shutting down service" line not until ~00:34:39, with FailNow at 5.56s — the third stage began essentially as the test's single window expired. Adds CLI.shutdownDeadline, set once when ctx is first observed done and threaded through shutdownBackends/lambdaCloseFn/shutdownServices, so the whole sequence honours one budget. A zero value means the HTTP server never observed cancellation (e.g. it failed to bind) and there is no clock to share. The test-side wait moves from a literal 5s to shutdownWaitTimeout (shutdownTimeout + 3s). This is a relaxation and worth naming as such: the wait was byte-for-byte equal to the production budget it was waiting on, so it lost to scheduling and channel-propagation overhead even when shutdown finished on time. The require.NoError branch is untouched, so a genuine shutdown error still fails the test. Reproduced pre-fix under 4-way and 15-way concurrent load; post-fix that failure mode is gone (0/32). A second, distinct defect was found and deliberately NOT fixed here: services/cloudfront and services/elbv2 each spawn a reconciler goroutine at construction and neither Handler implements service.Shutdowner, so their Close() is never called and every run() leaks two goroutines (measured: NumGoroutine 115->137 over 12 cycles). That is the leading explanation for the residual "context deadline exceeded" from server.Shutdown itself, which arrives fast rather than late and no test timeout can mask. Filed as gopherstack-7z8r rather than fixed inside this commit. Gates: go build ./... clean; go vet . clean; golangci-lint run . 0 issues. Closes gopherstack-becu Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
…utdown Both backends spawn a reconciler goroutine at construction (cloudfront/store.go runInvalidationReconciler, elbv2/store.go runHealthReconciler), and both have a working stopCh/Close(). But neither Handler implemented service.Shutdowner, and cli.go's shutdownServices only calls Shutdown on services satisfying that interface, so Close() was never reached. Every run() leaked two goroutines — measured at NumGoroutine 115->137 over 12 cycles. That affects anyone embedding or repeatedly starting the emulator, not just tests. Adds Shutdown(ctx) to both, patterned on the existing implementations in services/eks and services/networkmanager. elbv2's Handler.Backend is typed as the StorageBackend interface rather than a concrete type, so its Shutdown type-asserts a small optional `closer` interface instead — adding Close() to StorageBackend would mix lifecycle into a domain interface. Close() was already idempotent on both (guarded close(stopCh)), but neither JOINED the reconciler: Close() returned as soon as it signalled, so a caller could believe teardown was complete while the goroutine was still unwinding. That matters now that cli.go shares one deadline across teardown stages, so each backend adds a done channel closed by the reconciler on exit, and Close() waits on it. Regression tests type-assert any(h).(service.Shutdowner) — the exact mechanism shutdownServices uses — so they compile against the unfixed Handler and fail at runtime rather than at build time. Verified by neutering: renaming Shutdown so it no longer satisfies the interface leaves the package compiling and fails the test with "cloudfront.Handler must implement service.Shutdowner so cli.go's shutdownServices reaches it". Both tests are deliberately sequential with //nolint:paralleltest, since runtime.NumGoroutine() is process-wide and a parallel counter assertion is itself flaky. Swept for other constructors spawning goroutines: these two were the only ones. Others start reconcilers lazily via an explicit Start*/ensure* call, or use pkgs/worker.Group, whose Stop() already waits. The 46 worker.Group call sites were not individually audited for correct Shutdown wiring — that is a broader task worth its own pass. Gates: golangci-lint 0 issues; go build ./... ok; go test -race on both packages 3/3 runs; full go test ./services/... exit 0. Closes gopherstack-7z8r Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
…stack The regression tests added in 7c08b5b were themselves flaky and failed CI on the next run: services/cloudfront/shutdown_leak_test.go:60 "299" is not less than or equal to "298" Nothing leaked. They asserted a delta on runtime.NumGoroutine(), which is process-wide: the whole package runs in one binary under -race -shuffle on, so any unrelated goroutine starting or still unwinding between the two samples moves the count. A baseline of 298 shows how much else was in flight; the +1 was noise. Making the tests sequential was not enough — sequential-within-the-package does not stop other goroutines in the same process from moving a global. Now counts only goroutines whose runtime.Stack frame names the reconciler (runInvalidationReconciler / runHealthReconciler), so unrelated activity cannot perturb it. Baseline drops from ~298 to 0, which is the point. goleak was considered and rejected: its own docs call VerifyNone incompatible with parallel tests for this same attribution problem. require.Eventually absorbs the brief window between the done channel closing and the goroutine unwinding off the stack — safe here because Eventually's own polling goroutine never matches the reconciler frame, which is exactly what made it unusable against NumGoroutine(). Both properties that made these tests worth having are preserved, and I verified each by neutering rather than trusting the report: Shutdown renamed so it no longer satisfies service.Shutdowner — package still compiles, test fails with "cloudfront.Handler must implement service.Shutdowner so cli.go's shutdownServices reaches it". Shutdown kept but emptied so Close() is never called — package still compiles, test fails with "invalidation reconciler goroutine leaked after 12 construct/shutdown cycles (baseline=0)". The second is the one that matters: it proves the test detects a real leak, not merely a missing interface. No production file has a net diff; the 7z8r fix is unchanged. Gates: golangci-lint 0 issues; 12/12 passes under CI's own flags (-race -shuffle on -short -timeout 5m), since passing once is how the original shipped. Closes gopherstack-ndss Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
…loor TestHealthCmd_Success and TestLocalstackCompatibilityEndpoints failed with "context deadline exceeded" from server.Shutdown itself — the require.NoError branch, not the test's timeout branch. I suspected my own 3de7de0 (sharing one deadline across the three teardown stages) had converted a slow-but-successful shutdown into a returned error. That was wrong. Stage 1's effective budget is unchanged: the shared deadline is computed as time.Now().Add(shutdownTimeout) at the instant ctx.Done fires, which is what WithTimeout did before. Stage timings confirm stages 2 and 3 are irrelevant here — lambdaCloseFn runs in 27-70us and shutdownServices in 280us-1.3ms, every time. Stage 1 either finishes in ~80-200us or consumes the entire budget. The real cause is in the standard library. net/http/server.go:3309: // Issue 22682: treat StateNew connections as if // they're idle if we haven't read the first request's // header in over 5 seconds. if st == StateNew && unixSec < time.Now().Unix()-5 { A connection accepted but never read from is not closable by Shutdown until it is more than 5 seconds old. That constant is unexported and unconfigurable. shutdownTimeout was also exactly 5s, so any such connection raced the stdlib's own floor with zero margin, decided by scheduler jitter — which is why it was intermittent and load-dependent. A goroutine dump plus a ConnState hook confirmed the stalling connection sits in StateNew, never reaching active or idle, until forcibly closed. Both tests poll /health under require.Eventually, so a pooled connection racing shutdown is ordinary, not pathological. A real caller polling health as SIGTERM arrives could hit the same thing. 8s clears the stdlib floor's worst case (~5s + up to 1s of Unix-second truncation slop + 500ms poll granularity) with margin; measured stalls topped out at 5.34s over ~190 runs under concurrent -race load. shutdownWaitTimeout in cli_test.go derives from this constant and was not touched. 3de7de0 stands: it fixed a real stacking bug, and widening the test-side wait let this pre-existing defect surface as a distinct error instead of being swallowed into the same "did not shut down" bucket. Reverting it would re-merge the two failure modes. Before: 8 failures / 94 runs. After: 0 / 144, and 0/10 locally here. Separately found and filed rather than fixed: six services (directconnect, mgn, outposts, lightsail, resiliencehub, grafana) wire worker.Group Stop into Close() but their Handler lacks Shutdowner, so it is never called — the same class as gopherstack-7z8r. Gates: golangci-lint run . 0 issues; go vet . clean; go build ./... ok. Closes gopherstack-s01e Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
directconnect, mgn, outposts, lightsail, resiliencehub and grafana each
construct a pkgs/worker.Group and correctly wire Close() to work.Stop(), but
no Handler implemented service.Shutdowner. cli.go's shutdownServices only
calls Shutdown on services satisfying that interface, so Close() was never
reached. Same class as gopherstack-7z8r, and the fix is copied from it:
func (h *Handler) Shutdown(_ context.Context) { h.Backend.Close() }
var _ service.Shutdowner = (*Handler)(nil)
All six were verified against the three criteria before being touched, since
the list came from an audit rather than direct inspection. All six matched:
Group constructed in NewInMemoryBackend, Close() is exactly work.Stop(), no
pre-existing Shutdown. worker.Group.Stop() does cancel + wg.Wait(), so it
genuinely joins.
The bug is a different flavour from cloudfront/elbv2, and the issue I filed
described it wrongly. None of these six ever call Group.Go or Group.Ticker —
they use only Group.After, one-shot timers. So there is no persistent
goroutine sitting blocked; what leaks is scheduled state transitions firing
and mutating backend state after the service was supposedly shut down.
directconnect/bgpfailover.go:70 schedules on a caller-supplied minute-scale
duration, so a timer can fire long after teardown.
That also ruled out the stack-frame counting used for cloudfront/elbv2: a
timer callback is not a goroutine until it fires, and then it exits in under
a microsecond, so matching on runtime.Stack would be racy — the exact
flakiness class of gopherstack-ndss. Instead each test arms a probe timer on
the backend's Group after Shutdown and asserts it never fires, which works
because Group.After no-ops once stopped (pkgs/worker/group.go:149). Verified
under testing/synctest, matching pkgs/worker's own
TestGroupAfterIsNoOpAfterStop.
Each test keeps the any(h).(service.Shutdowner) assertion, so it compiles
against an unfixed Handler and fails at runtime rather than at build time.
Both neuters were run per package; I re-ran the second myself on
directconnect, which is the one that proves the leak check rather than the
interface assertion:
probe timer fired after Shutdown: worker.Group was never stopped, so
scheduled-transition timers leak past service shutdown
ArmProbeTimerForTest is added via export_test.go in each package — the
repo's existing idiom for reaching an unexported field from an external test
package, already used for RenewalIdempotencyLenForTest and
SeedOperationForTest. The two pre-existing export_test.go files were
appended to only; nothing they already exported changed. These files are
excluded from go build, so nothing reaches the production API.
Gates: golangci-lint on all six 0 issues; go test -race on all six ok;
10/10 under CI's flags (-race -shuffle on -short); go build ./... clean.
Closes gopherstack-s1ho
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
integration-tests (1) failed once on TestIntegration_Kinesis_EnhancedFanOut:
kinesis_test.go:369
read tcp [::1]:54726->[::1]:32770: use of closed network connection
Investigated and found no isolable defect. Recording the evidence and adding
coverage rather than changing behaviour.
The handler bounds a subscription two ways (handler_consumers.go:279-288): a
5-minute hard deadline, matching the SDK's own "for up to 5 minutes"
(api_op_SubscribeToShard.go:22), and an idle-close after 3 empty 200ms polls.
The idle path returns nil from the handler, which finishes the chunked body
normally; the SDK's event-stream reader treats that as io.EOF and closes the
channel with no error set. "use of closed network connection" is a
net.OpError from reading an already-closed socket, which needs something more
abrupt than an ordinary handler return.
The polling emulation is disclosed, not hidden: PARITY.md already lists
"Enhanced fan-out SubscribeToShard real streaming cadence / HTTP2 push
semantics beyond the polling emulation" under deferred. It is also
load-bearing — consumers_test.go:86 TestSubscribeToShard_StreamClosesAfterIdle
drives the handler synchronously through a ResponseRecorder and would hang on
a 5-minute clock.
No in-range commit reaches this path. c56c2ff, 724ce9b and 6a69563 touch
Reset defaults and Lambda ESM ARN parsing, not handler_consumers.go, and the
shutdown work (3de7de0, cc4dae6, 9596bc1) does not touch kinesis at all
— kinesis has no worker.Group and was in neither Shutdowner list. The
container also serves continuously during the test, so teardown timing is moot.
The new test drives a real AWS SDK client over real TCP (httptest.NewServer
rather than the ResponseRecorder most kinesis tests use) through the same
sequence as the failing integration test, and asserts stream.Err() is nil
after the idle close. 8x plain, 25x under -race, plus whole-package runs under
GOMAXPROCS=2: zero failures. That is coverage for the graceful path, not a
reproduction of the CI failure.
Not verifiable here: no Docker, so the containerised network path where the
failure actually occurred was never exercised. The abrupt-close mechanism is
inferred, not observed.
gopherstack-j60e stays OPEN, not closed as fixed, and is linked to the
existing gopherstack-i8q7 (a kinesis SubscribeToShard flake reproduced once in
1500+ runs, still open after ~540 further executions with no repro). Note the
symptoms differ — i8q7 is a missing record, this is a connection error — so
they are siblings in the same area rather than confirmed duplicates.
Gates: golangci-lint ./services/kinesis/... 0 issues; go test -race 8/8.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Collaborator
Author
📊 Code Coverage Report
Tip This project maintains a minimum coverage threshold of 85%. Maintain or improve coverage on new code to ensure long-term stability. Last updated: Thu, 10 Sep 2026 15:13:41 GMT |
…ases CodeFactor is a required status check under repository ruleset 22245006, and it was the only non-passing check on PR #2452 — every other required context, including coverage, passes. Its one finding was "Complex Method" on tagCleanupCases: tags_delete_cleanup_test.go:22:1: cognitive complexity 50 of func `tagCleanupCases` is high (> 30) (gocognit) gopherstack-kf9j previously concluded this was advisory and recorded "do not fix". That rested on a factual error: it checked classic branch protection, which 404s for this repo, and missed the ruleset that requires CodeFactor. The issue has been corrected. The complexity was not inherent to the table. All 23 create closures repeated out, err := b.CreateXxx(...) if err != nil { return "", err } return out.XxxARN, nil so gocognit counted the same branch 23 times, each nested inside a composite literal and therefore weighted twice. Extracting it into one generic helper removes every one of those branches from the literal: func arnOrErr[T any](out T, err error, arn func(T) string) (string, error) topic_rule keeps its own if — CreateTopicRule returns only an error, so it needs a separate GetTopicRule to obtain the ARN — and thing_type's two-step deprecate-then-delete is untouched. Those are real variations, left as they are rather than forced through the helper. Not done, deliberately: no splitting the table into chunks, no scattering entries across files, no //nolint. Those game the metric and make the table worse, which is what kf9j was right to guard against; removing duplication is a different act from partitioning to dodge a count. All 23 cases survive unchanged — the extracted name lists are byte-identical before and after, and both consuming tests still run 23 subtests each. funlen still fires on this function under --no-config (249 lines > 60), but it is a raw line count, not complexity; it was not part of CodeFactor's finding, .golangci.yml:587-599 already excludes it for _test.go repo-wide, and it fires on 47 sites across this package. Left alone. Gates: gocognit finding gone; golangci-lint ./services/iot/... 0 issues; cyclop, gocyclo and dupl clean on this file; go test -race ok; go vet clean; 48 PASS lines (23 subtests x 2 parent tests, plus the parents). Closes gopherstack-kf9j Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per-service AWS parity audit of all 163 services, one service at a time, each
verified against the pinned
aws-sdk-go-v2source rather than from memory.What this is
Every service got an audit issue under epic
gopherstack-plq, covering fivedimensions: AWS behavior compliance, LocalStack parity, cross-service
integration, performance, and resource leaks. All 163 are closed.
Every confirmed bug carries its own bd issue and a regression test that was
proven to fail without its fix — each guard neutered individually by line
number, with the build confirmed still green so a compile error could not
masquerade as a passing proof.
Recurring bug classes
The sweep kept surfacing the same shapes, which is the useful output here:
row but not a side map keyed by the same identity, so a recreated resource
inherits the dead one's state. Worst case:
docdb'sDeleteDBClusterSnapshotleftsnapshotAttributesbehind, so a snapshotrecreated under a reused identifier inherited the previous one's
cross-account restore grants.
finding across the campaign.
(
vpclattice'sserviceArn), or a convenience behavior invented for anomitted required member (
ses's emptyPolicyNamesmeaning "returneverything").
(
sagemakerruntime'sBodyvsInputLocation), required members, andbidirectional field pairings (
transcribe'sShowSpeakerLabels/MaxSpeakerLabels).because
pkgs/store.Index.removeswaps the last element into the removedslot.
outposts:ListOutposts/ListSitesreturned thelive backend pointers
Table.Snapshothands out, then released the lockwhile the handler read them unlocked.
Verification
Every agent finding was re-derived independently before being committed: the
SDK citation re-read verbatim, the per-operation modeled error set extracted
directly from
deserializers.go, and the regression test re-run against aneutered guard. Several agent claims were corrected or rejected in the
process, and a few agent pushbacks against the brief were accepted as correct.
make bd-auditreports zero trailer mismatches and zero typo'd IDs across allcommits.
TestSnapshotVersionGuardis green and the persistence golden wasrefreshed only for additive field changes, never to silence a version bump.
Known limitations
available; agents reported this honestly rather than claiming clean. The
wire-shape and error-code work is the solid part.
PARITY.md's own documented convention —trust rows marked
okwhose files are unchanged sincelast_audit_commit—rather than re-deriving every operation.
unfixed, including permission boundaries never consulted in the IAM
enforcement path and
.syncstep-function tasks degrading tofire-and-forget. Those were scoped out of the audits, not resolved by them.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HuRbN6tdkW27u2PFP46N1m